TST, API: test case where edfapi C library not installed #12
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is so that we don't confused the read_edf.py module with the read_edf function that is part of the read_edf.py module. The documented public API was and remains `from eyelinkio import read_edf` , which imports the function, so this should not have any compat issues with existing user code, unless they explicitly are doing something like eyelinkio.edf.read_edf.some_function
scott-huberty
changed the title
…lled I had to resolve the new directory structure on main ( src/eyelinkio ) with my renaming of modules in this PR (i.e. eyelinkio/read.py )
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow up from #11 - I added a unit test with a monkey patch to test the case where the SR Research
edfapiC library is not installed on the users machine. The test checks that our informative error message is thrown as expected.As far as I could work it out, this required a change in one of the module names..
read_edf.py->read.py, because there is also a function namedread_edfthat lived inread_edf.py, and gets directly imported to theedfnamespace byeyelinkio.edf.__init__.py, causing theread_edffunction to overshadow the module of the same name.... e.g.So the
unittest.mock.patchI added in this PR ended up trying to patch the function, not the module, causing an error.And I do think it's better to differentiate the module namespace from the function anyways.
The documented public API was and still is
from eyelinkio import read_edf(which imports the function) so this change hopefully will not break anyones code.EDIT: An alternative approach would be to avoid importing the
read_edffunction into the same namespace as theread_edfmodule:https://github.com/scott-huberty/eyelinkio/blob/7f5e2e5ea819afbd6b761aa2b844c78010d8d805/src/eyelinkio/edf/__init__.py#L2C1-L2C36
But I still prefer the approach I've taken, because I think that it makes it easier to distinguish between the module and the function (now the module is
read.pyand the function isread_edf)